//cc @nodejs/modules-active-members

Discussion issue: nodejs/modules#458

@devsnek as mentioned in the issue comment, this PR is to start exactly those discussions, and is blocked on them.

@guybedford oh i see, I started writing my review before you posted that comment :) Glad to see we're on the same page 👍

await import.meta.resolve('./does-not-exist') -> Throws not found as the default Node.js resolver checks for existence.

Can we prevent that? It feels constraining when resolve has to know already if the URL can be loaded. E.g. we could return the resolution as if path extensions wouldn't be tried..? Basically I want to prevent that it becomes observable in node that .resolve('./foo') can actually check for foo's existence because the same code running elsewhere wouldn't realistically provide that info.

require.resolve already checks for existence and throws - while I'm not a fan of an exception there, the existence check is actually quite important for all the use cases of a resolver I'm aware of.

There would be nothing wrong with a custom resolver hook supporting resolving to files that don't exist and instead throw during getSource. That would be perfectly valid. But for the Node.js resolver against the file system it is currently very much a resolver feature.

If you're fine actually touching the file, it feels like you could just import it if you want to check that it's in fact reachable? E.g. throwing implies that if it doesn't throw, the file can be loaded. Which isn't necessarily true:

$ cat x.js
cat: x.js: Permission denied
$ node -e "require.resolve('./x.js')"
$ node -e "require('./x.js')"
internal/fs/utils.js:220
    throw err;
    ^

Error: EACCES: permission denied, open '/private/tmp/x.js'

It feels like it encourages people to assume that just because they could resolve it, there's some sort of guarantee that the returned string can be loaded.

If we want to move the existence check from the resolver to the instantiation phase, we can do that, but that should be considered a separate concern to this PR. Also someone would need to justify and create that PR before stability.

Right now we don't leak when exactly the exception happens to import, so it's just an implementation detail. I would consider leaking it a compat risk with the web since even if the web adds import.meta.resolve, I assume it won't be supporting existence checks. If we get signals from the web that they will include existence, then that changes this whole thing. :)

@jkrems it is definitely not an option to import() a file, and risk evaluating code, to check for existence - that's a big part of the point of a resolver, to know it will work before trying it.

Right, but that’s not actually possible to derive from the resolution result. As shown above, file permissions are enough for a resolved path not to be valid for import. And this doesn’t touch things like race conditions or loaders generating files on-the-fly (or hiding them even though they exist on disk).

What is a use case where you need to know that you could require something but where you can’t try to actually require it?

That there are more reasons why an import might not succeed (including file permissions, transitive imports failing, exceptions) doesn't change that "the specifier doesn't resolve to something" is an important bit to know prior to attempting to evaluate it. One example in particular, this bit helps you catch a programmer error on the importing side using eslint-plugin-import, which is used in eslint-config-airbnb.

CI: https://ci.nodejs.org/job/node-test-pull-request/27911/

One example in particular, this bit helps you catch a programmer error on the importing side using eslint-plugin-import, which is used in eslint-config-airbnb.

I know (and actively use) that plugin. But if that plugin wanted to start using import.meta.resolve, couldn't it just stat the resulting file to see if it's there? It could then even fail if it wasn't readable etc..

My primary problem with leaking it is quite simple: It means that import.meta.resolve is very unlikely to be portable for minimal gain. I'd be surprised if browser vendors would be willing to roll a resource existence / reachability check into a function to resolve URLs. This smells like URL.equals doing DNS queries in Java.*

(*) Yes, node's default resolver accesses the file system. But one can implement the ES resolver using a "small" in-memory data structure that doesn't require knowledge about every single file on disk (an import map). If file existence is required to implement resolution, import maps are no longer sufficient to model import URL resolution in node. To me that's a big property to give up on for a pretty fuzzy check of "could this URL actually be fetched".

For illustration of why I wouldn't want resolve to throw in general: Adding a step that tries to access the resource is easy. Undoing that exception is hard: jkrems@6cebcb2#diff-6dda3ef5f860111452d324058e12a738R162-R183

How would your alternative handle different kinds of exceptional cases, like in that snippet? How would the API return a success or multiple kinds of error values on the same channel, and why would that be an improvement?

I assume the only actual error case you'd likely be interested in would be that the file doesn't exist. And that can be checked by stating it. Without running the entire loader hooks chain (including source hooks and/or transforms), we won't be able to return any of this info longer term if loader hooks are registered. So there would be a resolve function that loads files from disks and compiles them. Which just seems wrong.

I'm happy to support an async isLoadableAsCode(url): boolean API in addition to this that would do all these checks. But I want to be able to resolve to a directory. Or to a text file I'll never import. So even ignoring the weird side effects to the implementation of .resolve, it feels like an awkward API to have opinions on what the returned URL is used for.

If i have to manually stat it, then “there’s a filesystem” isn’t abstracted from users of the resolver. One of the big benefits is to abstract that away. I shouldn’t need any fs methods at all to resolve a specifier, just the abstraction.

If i have to manually stat it, then “there’s a filesystem” isn’t abstracted from users of the resolver. One of the big benefits is to abstract that away. I shouldn’t need any fs methods at all to resolve a specifier, just the abstraction.

I think I acknowledged that when I suggested that there could be a second function (isLoadableAsCode) that could do all the resource retrieval & content type testing required for more certainty. I just don't think that restricting import.meta.resolve to just this one case (only allow resolving to files that could likely be loaded via import as code) is worth it.

Especially because import.meta.resolve will unlikely be used by tools like eslint-plugin-import - because I assume for that plugin you wouldn't want to create an actual ES module with a faked URL inside of a node instance that has the proper content types supported you would need for the configuration of the plugin.

i think this argument may be a bit too isolated on the specific semantics of resolve. taking a step back, we have a pipeline somewhat like this:

import() => (specifier, referrer) => resolve() => url => getSource() => ...

The URL given to getSource() needs to be final. that is, getSource() understands how to load a found resource, but it does not understand how to find that resource.

Any behaviour needed to go from (specifier, referrer) to url should therefore be performed during resolve(). This may be as simple as new URL(specifier, referrer).toString() or it may be more complex, such as what our default resolver does.

The constraints of this model dictate that it must be assumed that resolve() can fail. For example, in our default resolver, missing permissions may preclude the read operation on a package.json, which means the algorithm doesn't know where to look for the entry file. The only reasonable behaviour is to bail.

For example, in our default resolver, missing permissions may preclude the read operation on a package.json, which means the algorithm doesn't know where to look for the entry file. The only reasonable behaviour is to bail.

I think that's fair. There's definitely exceptional circumstances where resolution cannot determine what the final url would be. I think throwing errors in that case would be the right thing to do.

What else is needed for us to land this?

What else is needed for us to land this?

I think this is ready to land unless Guy protests.

CI: https://ci.nodejs.org/job/node-test-pull-request/28891/

CI: https://ci.nodejs.org/job/node-test-pull-request/28892/

CI: https://ci.nodejs.org/job/node-test-pull-request/28896/

CI: https://ci.nodejs.org/job/node-test-pull-request/28897/

Landed in 0f96dc2.

The signatures of import.meta.resolve(…) and require.resolve(…) differ in terms of which arguments they accept: #31861.